feat(workflows): add closed-PR comment redirect by aidandaly24 · Pull Request #1328 · aws/agentcore-cli

aidandaly24 · 2026-05-20T20:36:50Z

Description

Add a workflow that responds to comments on closed PRs by external users — posts a redirect asking them to open a new issue, and pages oncall via Slack so closed-PR comments are not missed.

Also hardens the existing slack-issue-notification.yml to use injection-safe env: + toJSON() and emits a uniform 20-key payload (with event_type discriminator) so the Slack workflow can branch on event type.

Why: COE AI-7 (Bedrock AgentCore SDK SPII OTEL leak). An external user reported the v1.4.8 SPII leak by commenting on a closed revert PR; oncall does not monitor closed-PR comments, so the report was missed for ~20 hours. This closes that detection gap.

Quip: https://quip-amazon.com/SmCwABMBwzgH

Related Issue

Documentation PR

N/A (workflow-only change, no user-facing docs)

Type of Change

Testing

How have you tested the change?

I ran `npm run test:unit` and `npm run test:integ`
I ran `npm run typecheck`
I ran `npm run lint`
If I modified `src/assets/`, I ran `npm run test:update-snapshots` and committed the updated snapshots

Workflow-only change — no app-code tests apply. Manual test plan once merged:

Open and close a test PR; comment as a non-maintainer; verify bot posts redirect comment AND Slack alert fires.
Comment as a maintainer; verify no redirect, no alert.
Open a real issue; verify existing Slack alert behavior (regression check).

Checklist

I have read the CONTRIBUTING document
I have added any necessary tests that prove my fix is effective or my feature works
I have updated the documentation accordingly
I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
My changes generate no new warnings
Any dependent changes have been merged and published

Slack-side change (out of band)

The shared Slack workflow `GitHub Issue Response - Moab` was updated separately to:

Add `event_type`, `pr_number`, `pr_title`, `pr_url`, `pr_author`, `pr_state`, `pr_closed_at`, `pr_merged_at`, `comment_id`, `comment_url`, `comment_author`, `comment_body` as trigger variables.
Branch on `event_type` to render closed-PR-comment alerts differently from issue-opened alerts.
Page `@moab-primary-oncalls` for closed-PR comments.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Add closed-pr-comment.yml: detects comments on closed PRs by non-maintainers, posts a redirect to the issue tracker, and pages oncall via the existing SLACK_WEBHOOK_URL. Update slack-issue-notification.yml: switch to injection-safe env+toJSON pattern and emit a uniform 20-key payload (with event_type discriminator) so the Slack workflow can branch on event type. COE AI-7: an external user reported the SDK SPII OTEL leak by commenting on a closed revert PR; oncall does not monitor closed-PR comments, so the report was missed for ~20 hours.

github-actions · 2026-05-20T20:37:43Z

Package Tarball

aws-agentcore-0.14.1.tgz

How to install

gh release download pr-1328-tarball --repo aws/agentcore-cli --pattern "*.tgz" --dir /tmp/pr-tarball
npm install -g /tmp/pr-tarball/aws-agentcore-0.14.1.tgz

agentcore-cli-automation

Two correctness concerns around the dedup logic and oncall paging volume — see inline comments. The env/toJSON hardening on the existing notification workflow looks good.

agentcore-cli-automation · 2026-05-20T20:39:13Z

+          script: |
+            // PRs surface as `issue` events; merged_at is null when closed-not-merged.
+            const mergedAt = context.payload.issue.pull_request &&
+              context.payload.issue.pull_request.merged_at;


Slack notification is not deduped per PR/commenter, so oncall will be paged on every external comment.

The Post redirect comment step is gated by steps.existing.outputs.already_posted != 'true' (correct — we don't want to spam the PR with redirect replies), but Notify Slack is gated only on steps.perm.outputs.skip != 'true'. The result is:

A back-and-forth thread from one external user on a closed PR pages oncall on every comment.

A single user spamming a closed PR with N comments produces N pages.

Once the redirect comment has been posted, subsequent comments still page (despite the redirect having explicitly told them to open an issue).

Given this workflow exists specifically to close a detection gap, some over-paging is probably acceptable, but unbounded paging from a single thread is not. Options:

Also gate Notify Slack on steps.existing.outputs.already_posted != 'true' — pages once per (PR, external commenter) cohort, but risks missing genuinely new info posted after the redirect.

Keep the current behavior on the GitHub side and dedup on the Slack workflow side (e.g., suppress alerts where comment_author already alerted on the same pr_number within N hours). This is mentioned as a side-channel change, so the hook is already there.

Track per-commenter dedup with a second marker comment scoped to the commenter login (similar to how existing works, but keyed on comment.user.login).

Whichever you pick, please document the chosen behavior in the comment block above this step — right now the comment says "Slack alert is the load-bearing part" but doesn't address repeat-comment volume.

agentcore-cli-automation · 2026-05-20T20:39:13Z

+  issues: read
+
+jobs:
+  redirect:


Missing concurrency control makes the marker-comment dedup racy.

The Check for existing redirect comment step relies on a marker () being present in a previously posted comment. If two external comments arrive within the time it takes for the first workflow run to post its redirect (a few seconds), both runs will see no marker and both will post a redirect comment — defeating the dedup.

This is a realistic scenario: someone posts two follow-up comments back-to-back, or two different external users comment around the same time.

Suggest adding a per-PR concurrency group at the workflow or job level, e.g.:

concurrency: group: closed-pr-comment-${{ github.event.issue.number }} cancel-in-progress: false

This serializes runs for the same PR so the marker check is reliable. cancel-in-progress: false is important here — we still want the second run to execute (to page Slack for the second comment), just after the first has finished posting its marker.

agentcore-cli-automation · 2026-05-20T20:39:14Z

+          PR_CLOSED_AT: ${{ github.event.issue.closed_at }}
+          PR_MERGED_AT: ${{ github.event.issue.pull_request.merged_at }}
+          COMMENT_ID: ${{ github.event.comment.id }}
+          COMMENT_URL: ${{ github.event.comment.html_url }}


pr_state env var will be empty in the payload because of step ordering / env: evaluation.

The env: block on this step references ${{ steps.pr_state.outputs.state }}, which is fine — that step runs before this one. But please double-check by running the workflow once: in some env: evaluation paths, steps.<id>.outputs.<x> is only available within with:/run: and not in step-level env:. If it does come through empty, the fix is either:

Inline the expression directly in the payload: pr_state: "${{ steps.pr_state.outputs.state }}" (safe — values are merged/closed, not user input).

Promote pr_state to a job-level output and reference it via needs/jobs (overkill for a single job).

Not a blocker if you've verified it works end-to-end on a test repo, but worth confirming since the manual test plan is post-merge.

agentcore-devx-automation · 2026-05-20T20:40:14Z

Claude Security Review: no high-confidence findings. (run)

github-actions · 2026-05-20T20:40:23Z

Coverage Report

Status	Category	Percentage	Covered / Total
🔵	Lines	35.46%	10925 / 30803
🔵	Statements	34.8%	11604 / 33341
🔵	Functions	30.08%	1830 / 6082
🔵	Branches	29.38%	6900 / 23483

Generated in workflow #3425 for commit 856d525 by the Vitest Coverage Report Action

- Gate Slack notification on already_posted so oncall is paged only on the first external comment per PR. Subsequent comments don't page — the redirect comment has already directed the commenter to issues. - Add per-PR concurrency group so the marker-comment dedup is race-free (cancel-in-progress: false to ensure later runs still execute against the just-posted marker). - Inline steps.pr_state.outputs.state in the Slack payload directly rather than routing it through env: to avoid step-level evaluation ambiguity. The value is enum-typed (merged|closed) so injection-safe.

agentcore-devx-automation · 2026-05-20T20:47:33Z

Claude Security Review: no high-confidence findings. (run)

agentcore-cli-automation

Reviewed the latest revision (57a2506). All three concerns from the prior automated review (Slack paging volume, marker-comment race, pr_state step-output evaluation) have been addressed in this commit:

Notify Slack is now gated on steps.existing.outputs.already_posted != 'true', so paging is bounded to the first external comment per PR.
A workflow-level concurrency group keyed on github.event.issue.number with cancel-in-progress: false makes the marker-comment dedup race-free.
pr_state is inlined directly in the Slack payload ("${{ steps.pr_state.outputs.state }}"), avoiding any step-level env: evaluation ambiguity. The value is enum-typed (merged|closed), so injection-safe.

No new blocking issues found. Schemas in both workflows match (20 keys, event_type discriminator), attacker-controlled fields (*_TITLE, *_BODY) route through env: + toJSON(), the job-level if correctly filters to closed PRs and excludes bot comments (so the bot's own redirect reply won't recurse), and the permission-lookup catch defaults to "non-maintainer" which is the right safe default.

Looks good to merge.

tejaskash

All three prior review comments addressed. LGTM.

Hweinstock · 2026-05-21T13:15:43Z

+name: Closed PR Comment Redirect
+
+# COE AI-7: alert users who comment on closed PRs to open a GitHub issue.
+# An external user reported the v1.4.8 SPII leak by commenting on the


i'm not sure we want to be publishing details of the COE publicly.

Hweinstock · 2026-05-21T16:49:12Z

+            core.setOutput('state', mergedAt ? 'merged' : 'closed');
+
+      - name: Notify Slack
+        # Page oncall only on the FIRST external comment per PR (gated by


i dont think we should use internal language in the public repo.

Hweinstock · 2026-05-21T16:51:19Z

+              context.payload.issue.pull_request.merged_at;
+            core.setOutput('state', mergedAt ? 'merged' : 'closed');
+
+      - name: Notify Slack


is it possible to use https://github.com/aws/agentcore-cli/blob/main/.github/workflows/slack-issue-notification.yml, as a central place to control github --> slack integration? My impression is that we have some logic here and there that rely on eachother.

… file Address review feedback: - Merge closed-pr-comment.yml and slack-issue-notification.yml into a single github-slack-notifications.yml with two triggers (issues + issue_comment) and two jobs. One central place to control the GitHub -> Slack integration, as requested in review. - Remove internal incident references from the public workflow; the header now describes what the workflow does, not why it was created. No behavior change: both jobs emit the same 20-key payload the Slack workflow already consumes, gated and deduped exactly as before.

aidandaly24 · 2026-06-02T14:47:58Z

Thanks for the review @Hweinstock — addressed all three in 856d525:

Public COE details — removed all internal incident references (COE/SPII/version/timing) from the workflow. The header now describes only what the workflow does, not the incident that motivated it. That context lives in the internal doc + PR description instead.
Internal language in the Notify step — reworded the comment blocks to neutral language ("notify oncall" rather than "close the COE detection gap").
Central GitHub → Slack control point — consolidated closed-pr-comment.yml and slack-issue-notification.yml into a single github-slack-notifications.yml. It has two triggers (issues + issue_comment) and two jobs feeding the one shared Slack workflow. No behavior change — both jobs emit the same 20-key payload, gated/deduped exactly as before. Now there's one file that owns the GitHub→Slack integration, as you suggested.

agentcore-devx-automation · 2026-06-02T14:48:31Z

Claude Security Review: no high-confidence findings. (run)

Hweinstock

LGTM

Hweinstock · 2026-06-02T15:19:44Z

+                owner: context.repo.owner,
+                repo: context.repo.repo,
+                issue_number: context.payload.issue.number,
+                per_page: 100,


This may fail on PRs with > 100 comments, but I think that's an unlikely edge case.

github-actions Bot added the size/m PR size: M label May 20, 2026

aidandaly24 temporarily deployed to e2e-testing May 20, 2026 20:37 — with GitHub Actions Inactive

github-actions Bot added the agentcore-harness-reviewing AgentCore Harness review in progress label May 20, 2026

agentcore-devx-automation Bot added the claude-security-reviewing Claude Code /security-review in progress label May 20, 2026

aidandaly24 marked this pull request as ready for review May 20, 2026 20:38

aidandaly24 requested a review from a team May 20, 2026 20:38

agentcore-cli-automation suggested changes May 20, 2026

View reviewed changes

github-actions Bot removed the agentcore-harness-reviewing AgentCore Harness review in progress label May 20, 2026

agentcore-devx-automation Bot removed the claude-security-reviewing Claude Code /security-review in progress label May 20, 2026

github-actions Bot added size/m PR size: M and removed size/m PR size: M labels May 20, 2026

aidandaly24 temporarily deployed to e2e-testing May 20, 2026 20:43 — with GitHub Actions Inactive

agentcore-devx-automation Bot added the claude-security-reviewing Claude Code /security-review in progress label May 20, 2026

github-actions Bot added the agentcore-harness-reviewing AgentCore Harness review in progress label May 20, 2026

aidandaly24 changed the title ~~feat(workflows): add closed-PR comment redirect for COE AI-7~~ feat(workflows): add closed-PR comment redirect May 20, 2026

agentcore-devx-automation Bot removed the claude-security-reviewing Claude Code /security-review in progress label May 20, 2026

agentcore-cli-automation approved these changes May 20, 2026

View reviewed changes

github-actions Bot removed the agentcore-harness-reviewing AgentCore Harness review in progress label May 20, 2026

tejaskash previously approved these changes May 21, 2026

View reviewed changes

Hweinstock reviewed May 21, 2026

View reviewed changes

aidandaly24 dismissed tejaskash’s stale review via 856d525 June 2, 2026 14:47

github-actions Bot added size/m PR size: M and removed size/m PR size: M labels Jun 2, 2026

aidandaly24 had a problem deploying to e2e-testing June 2, 2026 14:48 — with GitHub Actions Failure

agentcore-devx-automation Bot added the claude-security-reviewing Claude Code /security-review in progress label Jun 2, 2026

agentcore-devx-automation Bot removed the claude-security-reviewing Claude Code /security-review in progress label Jun 2, 2026

Hweinstock approved these changes Jun 2, 2026

View reviewed changes

aidandaly24 merged commit a84cee4 into main Jun 2, 2026
28 of 29 checks passed

aidandaly24 deleted the feat/coe-ai7-closed-pr-comment-redirect branch June 2, 2026 18:03

Conversation

aidandaly24 commented May 20, 2026

Description

Related Issue

Documentation PR

Type of Change

Testing

Checklist

Slack-side change (out of band)

Uh oh!

github-actions Bot commented May 20, 2026

Package Tarball

How to install

Uh oh!

agentcore-cli-automation left a comment

Choose a reason for hiding this comment

Uh oh!

agentcore-cli-automation May 20, 2026

Choose a reason for hiding this comment

Uh oh!

agentcore-cli-automation May 20, 2026

Choose a reason for hiding this comment

Uh oh!

agentcore-cli-automation May 20, 2026

Choose a reason for hiding this comment

Uh oh!

agentcore-devx-automation Bot commented May 20, 2026

Uh oh!

github-actions Bot commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report

Uh oh!

agentcore-devx-automation Bot commented May 20, 2026

Uh oh!

agentcore-cli-automation left a comment

Choose a reason for hiding this comment

Uh oh!

tejaskash left a comment

Choose a reason for hiding this comment

Uh oh!

Hweinstock May 21, 2026

Choose a reason for hiding this comment

Uh oh!

Hweinstock May 21, 2026

Choose a reason for hiding this comment

Uh oh!

Hweinstock May 21, 2026

Choose a reason for hiding this comment

Uh oh!

aidandaly24 commented Jun 2, 2026

Uh oh!

agentcore-devx-automation Bot commented Jun 2, 2026

Uh oh!

Hweinstock left a comment

Choose a reason for hiding this comment

Uh oh!

Hweinstock Jun 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions Bot commented May 20, 2026 •

edited

Loading